New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update: Code path analysis for class fields (fixes #14343) #14886
Conversation
So I took a look at the code path analysis for a similar pattern: function Foo() {
this.a = b ? c : d
};
new Foo() The result is:
Which results in the following dot graphs: Similar to class fields, class Foo {
a = b ? c : d;
}
new Foo(); So do we have a bug in our existing code path analysis? Or are the class fields different in some way that I'm not seeing? (Again, I really don't understand how any of this works, so please feel free to point out how I'm misunderstanding this.) |
A separate code path (which we currently create only for functions, incl. methods) indicates that the code inside isn't executed in the same flow as the surrounding code, but we don't track function calls to connect code paths. That info isn't provided by our code path analysis, so A;
class B extends C {
[D] = E + F;
}
G; A -> B -> C -> D -> G is one execution flow, they're all evaluated one after another. AST traversal also goes through The specification defines initializers as functions https://tc39.es/ecma262/#sec-runtime-semantics-classfielddefinitionevaluation. For example: class Foo {
x = 0;
y = this.x + 1;
} is roughly the same as: function xInitializer() {
return 0;
}
function yInitializer() {
return this.x + 1;
}
class Foo {
constructor() {
Object.defineProperty(this, "x", { value: xInitializer.call(this) /*, ... */ });
Object.defineProperty(this, "y", { value: yInitializer.call(this) /*, ... */ });
}
} I don't think our code path analysis can provide too advanced info about the execution flow of class fields, like the one proposed in #14317, but it seems reasonable to at least create a separate code path for each class field initializer, i.e. to treat |
Gotcha. That makes sense, thanks for explaining. I'm still not sure I know how to do that, but I'll dig in a bit. |
A note for myself: The challenge here is to treat the right side of |
This definitely looks like the right direction! It could be actually already correct although it doesn't appear so. Maybe just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This works as I would expect it to.
The only thing that might be confusing and difficult to handle in rules is that for code such as class Foo { a = () => {} }
the onCodePathStart
is raised twice with the same node
argument (the arrow function), so we might want to add CodePath#type
property which could be used to make a distinction between the initializer code path and function's own code path.
Can you explain this a bit more? I'm not understanding what you're suggesting here. |
My suggestion is that we add Rules are using code path analysis through events, like Before introducing class fields, it was clear what each code path refers to, by With class fields initializers, which don't have a dedicated wrapper node in the AST, class Foo {
a = () => {};
} For the above code, |
Ah gotcha, thanks for explaining. That sounds like a good way to differentiate the types of code paths. I’ll work on that. |
I added an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! origin
makes sense to me.
LGTM, I left only two minor notes.
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Merged, thanks for taking on this task! |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
What changes did you make? (Give an overview)
I added tests for code path analysis to show that analysis is working correctly for class fields. No other changes were necessary as I believe the standard traversal gives the code path analyzer all of the information it needs.
I started by adding a test that showed how we currently traverse an object literal with a property that is initialized by a conditional statement because this is the closest analog to how code path analysis should work with class fields.
I then added a test showing that a class field initialized with a conditional statement creates the correct code path, as it basically matches what happened in the object literal case.
Is there anything you'd like reviewers to focus on?
Am I missing anything?